Skip to content

Fall back to direct subtype comparison at the end in dropIfSuper and dropIfSub #20523

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jun 19, 2024
Merged

Fall back to direct subtype comparison at the end in dropIfSuper and dropIfSub #20523

merged 2 commits into from
Jun 19, 2024

Conversation

noti0na1
Copy link
Member

@noti0na1 noti0na1 commented Jun 4, 2024 •
edited
Loading

Improve compiling in #20516.

We need to be careful to check branches of AndTypes and OrTypes in correct order, see discussion in issue #20516.

Fall back to direct subtype comparison at the end in dropIfSuper and dropIfSub.

We may need to improve subtype checking for large intersections further to be able to fully test the example.

Currently, tests/pos-deep-subtype/i20516.scala compiles in 5s with this PR, and in 40s on the main branch.

@noti0na1 noti0na1 marked this pull request as ready for review June 10, 2024 16:26
@noti0na1
Copy link
Member Author

test performance please

@dottybot
Copy link
Member

performance test scheduled: 1 job(s) in queue, 0 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/20523/ to see the changes.

Benchmarks is based on merging with main (dea3d10)

@noti0na1 noti0na1 requested a review from odersky June 10, 2024 19:06
odersky
odersky requested changes Jun 18, 2024
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look reasonable, but we should check they don't introduce a performance regression elsewhere. Have a look at the discussion #20034. Can you run the test cases mentioned there and check whether they become significantly slower?

@odersky odersky assigned noti0na1 and unassigned odersky Jun 18, 2024
@noti0na1
Copy link
Member Author

Can you run the test cases mentioned there and check whether they become significantly slower?

@odersky I compared the compiling time of i19907_*.scala, i12915.scala, i7034.scala, and other tests in deep-subtype between my fix and the main branch, and there is no difference.

@noti0na1 noti0na1 assigned odersky and unassigned noti0na1 Jun 19, 2024
odersky
odersky approved these changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, looks all good then.

@noti0na1 noti0na1 merged commit 1f00e4a into scala:main Jun 19, 2024
@noti0na1 noti0na1 deleted the fix-20516 branch June 19, 2024 13:57
@noti0na1 noti0na1 mentioned this pull request Jun 19, 2024
Closed
@Gedochao Gedochao mentioned this pull request Jun 27, 2024
Closed
@Kordyjan Kordyjan added this to the 3.5.1 milestone Jul 3, 2024
@Gedochao Gedochao mentioned this pull request Jul 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odersky odersky odersky approved these changes

Assignees

@odersky odersky

Labels
None yet
Projects
None yet
Milestone
3.5.1
Development

Successfully merging this pull request may close these issues.
4 participants
@noti0na1 @dottybot @odersky @Kordyjan